refactor(network): Remove dynamic allocations for NetPacket by Caball009 · Pull Request #2866 · TheSuperHackers/GeneralsGameCode

Caball009 · 2026-07-09T18:58:50Z

This PR makes two changes to remove all dynamic allocations for NetPacket:

Refactors NetPacket::ConstructBigCommandList so that NetPacket doesn't have to be memory pooled.
Removes memory pool for NetPacket, so that it can be stack allocated.

greptile-apps · 2026-07-09T19:03:03Z

Greptile Summary

This PR removes the MemoryPoolObject base from NetPacket so it can be stack-allocated, and replaces ConstructBigCommandPacketList (which returned a list of heap-allocated NetPackets) with ConstructBigCommandList (which returns a NetCommandList directly). Most of the call-site cleanup in Connection.cpp and ConnectionManager.cpp is straightforward and correct.

Header / pool cleanup: NetPacket no longer inherits from MemoryPoolObject; pool entries are removed from both .inl files; constructor changed to const TransportMessage&. All clean.
Connection.cpp / ConnectionManager.cpp: Stack-allocated NetPacket instances replace heap allocations; the addMessage nullptr guard is now present. No regressions on these paths.
ConstructBigCommandList: All chunk NetCommandRefs share a single wrapperMsg pointer that is mutated in place each iteration. Because the deduplication logic in NetCommandList::addMessage compares ->getCommand() pointers, every chunk after the first is rejected as a duplicate of itself, leaving only the last chunk's data in the returned list. This silently corrupts any game command that spans two or more network chunks.

Confidence Score: 3/5

Not safe to merge: large game commands that require splitting across multiple network chunks will be silently truncated to only their last piece, breaking reassembly on the receiving end.

The ConstructBigCommandList rewrite shares one wrapperMsg object across all chunk refs. Because the object is mutated in-place each loop iteration, every previously-inserted ref immediately reflects the new state, causing NetCommandList::addMessage's deduplication check to reject every chunk after the first as a duplicate of itself. The surviving entry carries only the final chunk's data. Commands large enough to span two or more chunks — perfectly normal large game orders — will be corrupted on every send. The rest of the stack-allocation changes are clean.

Core/GameEngine/Source/GameNetwork/NetPacket.cpp — specifically the ConstructBigCommandList loop where wrapperMsg is shared across all chunk refs.

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameNetwork/NetPacket.cpp	Refactored `ConstructBigCommandPacketList` → `ConstructBigCommandList`. Contains a correctness regression: all chunk refs share the same `wrapperMsg` pointer, so only the last chunk survives the list's deduplication logic for any command requiring ≥2 chunks.
Core/GameEngine/Include/GameNetwork/NetPacket.h	Removes `MemoryPoolObject` base and related typedefs; changes constructor signature to `const TransportMessage&`. Clean header changes, no issues.
Core/GameEngine/Source/GameNetwork/Connection.cpp	Replaces pointer-based `NetPacket` usage with stack-allocated instances in `sendNetCommandMsg` and `doSend`. Logic is equivalent to the old code; `addMessage` nullptr guard added correctly.
Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp	Replaces heap-allocated `NetPacket` with a stack-local in `doRelay`; removes now-unnecessary end-of-function cleanup for `packet`/`cmdList`. The outer `cmdList` (from `getReadyCommands`) remains correctly scoped and deleted.
Core/GameEngine/Source/Common/System/GameMemoryInitPools_Generals.inl	Removes `NetPacket` pool entry, consistent with removing `MemoryPoolObject` inheritance.
Core/GameEngine/Source/Common/System/GameMemoryInitPools_GeneralsMD.inl	Removes `NetPacket` pool entry, consistent with removing `MemoryPoolObject` inheritance.

Sequence Diagram

sequenceDiagram
    participant C as Connection
    participant NCL as NetCommandList (returned)
    participant WM as wrapperMsg (single object)

    C->>NCL: ConstructBigCommandList(tempref)
    Note over NCL,WM: Loop iteration 1
    WM->>WM: setID(ID_1), setData(chunk0)
    NCL->>NCL: addMessage(chunkRef1 → wrapperMsg) ✓ inserted
    Note over NCL,WM: Loop iteration 2
    WM->>WM: setID(ID_2), setData(chunk1)
    Note over NCL,WM: chunkRef1 now also shows ID_2 (same object!)
    NCL->>NCL: addMessage(chunkRef2 → wrapperMsg) ✗ duplicate detected → deleted
    Note over NCL,WM: Loop iteration N
    WM->>WM: setID(ID_N), setData(chunkN-1)
    NCL->>NCL: addMessage(chunkRefN → wrapperMsg) ✗ duplicate detected → deleted
    NCL-->>C: list with 1 entry (last chunk data only)
    C->>C: m_netCommandList.addMessage(wrapperMsg) — only last chunk queued

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
Core/GameEngine/Source/GameNetwork/NetPacket.cpp:108-138
**All chunk refs share the same mutable `wrapperMsg` — big commands silently truncated to one chunk**

Every `chunkRef` created in the loop points to the *same* `wrapperMsg` object. Because `wrapperMsg` is mutated in place on each iteration (new command ID, new chunk number, new data), all refs that were already inserted into `commandList` immediately observe the new state. The deduplication logic in `addMessage(NetCommandRef*&)` compares `m_lastMessageInserted->getCommand()` and `msg->getCommand()`, but both are literally the same pointer, so every chunk after the first is discarded as a "duplicate" and its `chunkRef` is deleted. The list ends up with a single entry holding the last chunk's data and the last generated command ID.

The old code worked because `addCommand` serialized each chunk's state into a raw byte buffer inside a per-chunk `NetPacket`, and the caller then called `getCommandList()` to deserialize independent `NetCommandMsg` objects from those bytes. The new path eliminates that serialization round-trip, but must instead allocate a distinct `NetWrapperCommandMsg` per chunk (so each ref owns its own snapshot of the state) to preserve correctness.

Any game command large enough to span two or more network chunks will be silently dropped to its last piece, breaking reassembly on the receiver.

_{Reviews (8): Last reviewed commit: "Added nullptr check." | Re-trigger Greptile}

xezon · 2026-07-11T09:22:13Z

What is the motivation for removing memory pool? The main purpose of memory pool is cache and defrag friendlyness.

Caball009 · 2026-07-11T11:16:46Z

What is the motivation for removing memory pool? The main purpose of memory pool is cache and defrag friendlyness.

The purpose of a memory pool is to mitigate the downsides of frequent dynamic allocations. It's unnecessary if you don't need dynamic allocations.

xezon · 2026-07-11T14:56:47Z

But it is still allocated with NetPacket *packet = new(NetPacket);

Caball009 · 2026-07-13T13:30:55Z

But it is still allocated with NetPacket *packet = new(NetPacket);

Rewritten so that this is no longer the case.

Caball009 · 2026-07-13T13:32:35Z

@greptileai re-review this pull request.

xezon

I do not see fault.

xezon · 2026-07-24T17:42:29Z

Needs rebase and todo.

xezon · 2026-07-26T07:39:21Z

1 TODO and Bot complaint left.

xezon · 2026-08-01T15:08:49Z

This change was almost done

Caball009 · 2026-08-01T20:40:17Z

I was a bit puzzled by greptile's complaint, so figuring that one out is the hold up for now.

xezon · 2026-08-01T21:29:24Z

Hmm yes the bot has a point. wrapperMsg is the same pointer added to every new NetCommandRef, so they all see the same state.

Dismiss until wrapperMsg is fixed

xezon

Looks ok

xezon · 2026-08-06T07:10:40Z

 	UnsignedInt currentChunk = 0;
 	UnsignedInt bigPacketCurrentOffset = 0;

-	while (currentChunk < numChunks) {


Maybe add assert that num chunks shall not be zero. Or add an early return if it can be (bufferSize is 0).

xezon · 2026-08-06T17:30:35Z

 	// create the wrapper command message we'll be using.
 	NetWrapperCommandMsg *wrapperMsg = newInstance(NetWrapperCommandMsg);
 	// get the amount of space needed for the wrapper message, not including the wrapped command data.
 	UnsignedInt wrapperSize = GetBufferSizeNeededForCommand(wrapperMsg);


While at it, you can also remove the GetBufferSizeNeededForCommand function and unroll its content here.

xezon · 2026-08-06T17:34:28Z

 	NetWrapperCommandMsg *wrapperMsg = newInstance(NetWrapperCommandMsg);
 	// get the amount of space needed for the wrapper message, not including the wrapped command data.
 	UnsignedInt wrapperSize = GetBufferSizeNeededForCommand(wrapperMsg);
 	UnsignedInt commandSizePerPacket = MAX_PACKET_SIZE - wrapperSize;


Can rename to maxDataSizePerPacket

xezon · 2026-08-06T17:43:14Z

 	NetWrapperCommandMsg *wrapperMsg = newInstance(NetWrapperCommandMsg);
 	// get the amount of space needed for the wrapper message, not including the wrapped command data.
 	UnsignedInt wrapperSize = GetBufferSizeNeededForCommand(wrapperMsg);
 	UnsignedInt commandSizePerPacket = MAX_PACKET_SIZE - wrapperSize;


Maybe move this into a new (static) NetWrapperCommandMsg::getMaxDataLength, then ConstructBigCommandList can be simpler. If static, then the allocation of NetWrapperCommandMsg can also be simplified in this function, because it is only allocated here so early for getting the size without data.

Caball009 added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels Jul 9, 2026

Caball009 mentioned this pull request Jul 9, 2026

perf(network): Reduce loop complexity for transport messages and remove unused static variables #2659

Merged

1 task

Caball009 force-pushed the remove_network_netpacket branch 3 times, most recently from 9b43034 to e1fead4 Compare July 13, 2026 13:12

greptile-apps Bot reviewed Jul 13, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/NetPacket.cpp

Caball009 changed the title ~~refactor(network): Remove memory pool for NetPacket~~ refactor(network): Remove dynamic allocations for NetPacket Jul 13, 2026

Caball009 force-pushed the remove_network_netpacket branch from e1fead4 to 757d5cf Compare July 13, 2026 13:47

xezon previously approved these changes Jul 14, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/NetPacket.cpp Outdated

Caball009 force-pushed the remove_network_netpacket branch from 757d5cf to e396279 Compare July 24, 2026 11:33

greptile-apps Bot reviewed Jul 24, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/Connection.cpp

Caball009 added 4 commits July 24, 2026 20:21

Refactored 'ConstructBigCommandPacketList'.

94cf9ae

Removed memory pool for NetPacket.

5c00a6c

refactor: moved the initialization of 'commandList'.

ad51ba1

Added nullptr check.

27c0487

Caball009 force-pushed the remove_network_netpacket branch from 3085796 to 27c0487 Compare July 24, 2026 19:36

greptile-apps Bot reviewed Jul 24, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/NetPacket.cpp Outdated

Addressed feedback - fixed reuse of wrapper message.

1f5578d

xezon approved these changes Aug 6, 2026

View reviewed changes

Uh oh!

Conversation

Caball009 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Uh oh!

xezon commented Jul 11, 2026

Uh oh!

Caball009 commented Jul 11, 2026

Uh oh!

xezon commented Jul 11, 2026

Uh oh!

Uh oh!

Caball009 commented Jul 13, 2026

Uh oh!

Caball009 commented Jul 13, 2026

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xezon commented Jul 24, 2026

Uh oh!

Uh oh!

xezon commented Jul 26, 2026

Uh oh!

xezon commented Aug 1, 2026

Uh oh!

Caball009 commented Aug 1, 2026

Uh oh!

xezon commented Aug 1, 2026

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

xezon Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Caball009 commented Jul 9, 2026 •

edited

Loading

greptile-apps Bot commented Jul 9, 2026 •

edited

Loading